[wip][LLD] Support RISCV vendor-specific relocations. #168497
Conversation
|
@lenary for feedback |
🐧 Linux x64 Test Results
|
lenary
left a comment
lenary
left a comment
There was a problem hiding this comment.
I like the iterator, it seems clean and a sensible wrapper for the logic.
I have more feedback about the bitfields, at the moment, but overall I'm happy with the direction so far.
I haven't yet had to think about dynamic relocations, and if they have some differences compared to the existing design/spec (are local symbols a problem here?)
| auto riscvVendorString = getRISCVVendorString(reloc.type); | ||
| if (ctx.arg.emachine == llvm::ELF::EM_RISCV && riscvVendorString) { | ||
| Symbol &vendorSym = *ctx.symtab->addSymbol(Defined{ | ||
| ctx, ctx.internalFile, *riscvVendorString, llvm::ELF::STB_GLOBAL, |
There was a problem hiding this comment.
| ctx, ctx.internalFile, *riscvVendorString, llvm::ELF::STB_GLOBAL, | |
| ctx, ctx.internalFile, *riscvVendorString, llvm::ELF::STB_LOCAL, |
I think we've said these need to be local, but maybe that's not right for dynamic relocs.
There was a problem hiding this comment.
I thought I recalled that dynamic relocations could only point to global symbols, but I'm failing to find that requirement in the ELF spec.
There was a problem hiding this comment.
Oracle docs seem to agree with you:
The .dynsym table begins with the standard NULL symbol, followed by the files global symbols. STT_FILE symbols are typically not present in this symbol table. STT_SECTION symbols might be present if required by relocation entries.
From: https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-79797.html#stlac
I will go back to the ABI about this. We made them local to echo mapping symbols, but evidently that doesn't work for vendor-specific dynamic relocations.
There was a problem hiding this comment.
Thanks. Can you share the ABI discussion link once you bring it there?
lenary
left a comment
lenary
left a comment
There was a problem hiding this comment.
This seems clear to me what's going on (though I've implemented this once already). I would be happy to help review/maintain this code if it was landed.
One Nit below.
Do we want to make sure we never parse a relocation from a file that's more than 255? They would have had to have been made deliberately, but are possible to encode on rv64. It's the sort of error that's useful to have and remove in the future, if the abi ever gets so many relocations.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
resistor
force-pushed
the
vendor-relocs
branch
from
458904f to
2b9e004
Compare
2 participants
This is an implentation of RISCV vendor-specific relocation support in LLD, derived from a working prototype implementation in the downstream CHERIoT repository: CHERIoT-Platform/llvm-project@3d6d6f7
At this time I am seeking design feedback on the shape of the implementation, while I work on developing tests that do not depend on CHERIoT.
Key elements of the design include: